[X11] Partial fix for Editor and Project Manager stealing focus on some window managers #86101
Conversation
akien-mga
changed the title
akien-mga
added
bug
platform:linuxbsd
topic:porting
cherrypick:4.2
|
What will be the consequence of removing that code on WMs which don't have that focus stealing bug? In other words, what is the original code meant to do? |
Tested briefly on KDE Plasma on X11, it doesn't reintroduce those issues for me. From a quick test I didn't spot any weird change in behavior. Would be good to have it tested on more DEs/WMs and on Xwayland. |
|
Thanks for taking the time to review this! I'm in code review at work this week, so I actually have some extra free time in the evenings until this weekend. I can install a lot of different DEs on my Arch box, so I will try to test this against some other popular DEs and WMs after work this week and report the results. :) |
|
As another random data point, I use i3 and Godot's "window grabbing focus behavior" is super disruptive. However, in my case, it doesn't lead to multiple windows fighting for focus and things getting locked up, it's just very annoying. :-) When I have a chance I'll try with this PR and see if it helps in my case. But it would be kind of nice to have an editor setting that I could enable that would just prevent any window in the editor from grabbing focus. |
In case it helps, #74378 is related to #68305, but has a broader scope, and users have reported odd focus behavior between workspaces on i3. I generally only use one workspace, so I haven't looked into this, but one user confirmed that this workaround does help with focus on i3 but doesn't fix the issue between workspaces. See #68305 (comment) for details. Confirmation from another i3 user would be really helpful though. :) |
|
I just tested this PR with i3, and things don't seem to get any worse, but it doesn't seem to help my problems either. To be clear, my issues mostly revolve around switching between the Godot editor and VS Code, where Godot will grab (or refuse to give up) input focus, and despite a new window visually appearing to have focus (like, the title bar is highlighted in the way that usually indicates it has focus), all the keyboard input still goes into the Godot editor. My "work around" is that I have to toggle back and forth between focusing on Godot and focusing on the thing I actually want to get my keyboard inputs, and eventually they'll go where I want. I'll try to see if my problem is represented in one of the existing issues, and if not, make a new issue. |
Yeah, there are a lot of problems with the focus behavior on WMs in the X11 backend. This PR is only intended to address the system lock up on very specific configurations since it seems like a fix for the other focus issues will require a significant overhaul of the event handlers, and a few devs have been working on this off and on for months without a lot of progress (mostly due to lack of time). I think some of the comments in #74378 mentioned similar issues with i3, and the user who tested this workaround on i3 previously did mention something similar about focus problems with an external text editor. Thanks for testing it out! |
|
Edit: Removed the other gifs as they all show the exact same behavior and they were causing the issue to load slowly and also burning my eyes. :) Okay, I was able to test this on the following WMs/DEs: Fluxbox with picom (Xwayland), Openbox X11, Openbox Xwayland, Budgie, Cinnamon, Enlightenment, lxde, lxqt over Fluxbox, lxqt over Openbox, Mate, and xfce4. I just did a quick test of launching the editor and opening a popup, so I have not tested for regressions related to #37674 and #41578 yet, but I wanted to make sure there wasn't anything weird going on with popups before regression testing on each DE. I will check for #37674 and #41578 on each DE eventually this week, but it probably won't be tonight. :) The following is a gif of the test on Fluxbox with an Xwayland compositor. The behavior was exactly the same on the other environments, so I am only including the single gif, but I can upload the others if you want.
|
|
Okay, I took a look at #37674 and #41578 and they were much simpler to test than I thought. 🤦♂️ I tested both on all of these environments: Fluxbox X11, Fluxbox Xwayland, Openbox X11, Openbox Xwayland, Budgie, Cinnamon, Enlightenment, lxde, lxqt over Fluxbox, lxqt over Openbox, Mate, and xfce4. The following is a gif of the test on Fluxbox X11. All of the other environments gave exactly the same behavior, so I am only uploading the single gif:
I believe this is everything I can test as far as different DEs goes. Please let me know if you have any questions for me. Thanks! |
There was a problem hiding this comment.
Tested locally (Fedora 39 KDE with X11, rebased against 96296e4), it doesn't improve the situation with regards to focus stealing on floating window managers. The issue described in #86441 (review) still occurs with this PR, unfortunately.
That said, with this PR cherry-picked on top of #86441, the issue reported in the other PR remains fixed. I tested the editor quickly and couldn't notice any regressions in editor popup behavior. As a result, we can still choose to merge this if it helps improve the situation on tiling WMs.
|
Just a heads up, I wanted to check and see if #86441 and #86671 fixed this issue as well, so I tested the latest master, but I still get the system lockup on my machine. Here's a recording of the behavior on current master:
Interestingly, the focus swap is slowed down considerably from before, but it's still faster than my 24fps screen recorder can keep up with, and I still can't get any input to register. I'm guessing the slower focus swapping is due to @dsnopek's fixes reducing the number of extraneous X11 events handled by Godot, but I can't say for sure, and it's still continuous. The patch provided in this PR still fixes the lockup on my system, so I think this PR is still necessary at this point. Lmk if there is anything else I can help with review-wise. Thanks! |
|
This needs a rebase against the current |
…window manager on Linux This is a workaround for the most critical portion of the WM focus bug described in #68305. On some specific X11 WM configurations, the editor's main window and any popups it creates will fight for focus, which causes a total system lockup due to mouse and keyboard input being stolen as well. Getting out of this infinite loop requires force restarting the system. It can be tested with the following shell script: ```bash !#/bin/sh godot4 & sleep 30 pkill -x godot4 ``` The workaround identified in #68305 is to remove the call to XSetInputFocus in the ConfigureNotify event handler, so I have removed the conditional block that calls this as well as the setup code above it since there is no need to allocate the memory for the variables if they won't be used in that call anymore. This is just a hack and is not a complete fix for #68305. Multiple developers are collaborating on a proper fix in the discussion in that issue, but time is a valuable resource that no one has enough of, so I am committing this workaround as a stop-gap to prevent the most critical problem while we work on a full solution for the underlying cause.
|
Rebased over d335281 Thanks! |
|
Thanks! |
This is a workaround for the most critical portion of the WM focus bug described in #68305. On some specific X11 WM configurations, the editor's main window and any popups it creates will fight for focus, which causes a total system lockup due to mouse and keyboard input being stolen as well. Getting out of this infinite loop requires force restarting the system.
It can be tested with the following shell script:
Here is a gif of the bug in action on my machine:
The gif above actually doesn't show the exact behavior. The constant focus swap is actually too fast for my screen recorder's framerate to pick up, so what you are seeing is about 24 out of 60 focus swaps. :)
Here is the same with this patch applied:
My system information:
The workaround identified by @nargacu83 in #68305 is to remove the call to XSetInputFocus in the ConfigureNotify event handler, so I have removed the conditional block that calls this as well as the setup code above it since there is no need to allocate the memory for the variables if they won't be used in that call anymore.
This is just a hack and is not a complete fix for #68305. Multiple developers are collaborating on a proper fix in the discussion in that issue, but time is a valuable resource that no one has enough of, so I am submitting this workaround as a stop-gap to prevent the most critical problem while we work on a full solution for the underlying cause.
This bug affects a very small number of users, and the general consensus in the related issue is that we don't want to submit a PR for a hack if we can get a proper fix. Particularly since we aren't completely sure why this workaround fixes the problem, and I agree with this as well, but personally, I am concerned that with this being a platform bug, it could affect exported games and not just developers, and in that case, it probably wouldn't show up on the developer's machine, but a player with the affected configuration could encounter the bug when running an exported Godot game, and although the number of people playing games on custom WM-only Linux machines is probably very low, it only takes one very vocal gamer on Twitter to ruin an indie studio, so I wanted to submit this workaround now instead of sitting on it since no one in the related issue has reported any regressions with this workaround, and we don't know when a proper fix will be ready.
Please feel free to close this PR if you feel that we should wait until we have a better idea of the problem and exactly why this workaround works. Thanks!